-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: correct return type annotation for find_one_and_* methods to include None
#2615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix: correct return type annotation for find_one_and_* methods to include None
#2615
Conversation
ShaneHarvey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this also a bug in the other find_one_and_* methods?
| comment: Optional[Any] = None, | ||
| **kwargs: Any, | ||
| ) -> _DocumentType: | ||
| ) -> Optional[_DocumentType]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As someone not familiar with the ins-and-outs of this repo, is Optional[<type>] preferred over <type> | None? Or is it that it needs to support python versions that don't have the | syntax? (Same comment goes for the other instance of this as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reviewers, Zach is my coworker looking over this PR before I mark it as ready for review.
pymongo supports Python 3.9+. See requires-python = ">=3.9" in pyproject.toml.
Support for the "|" syntax for a Union was added with Python 3.10 (PEP 604).
Regardless of what is/isn't supported/required, using Optional (rather than |) is more consistent with the rest of the code.
I just checked and it is. I'll update this PR to fix the type hints for those methods shortly. |
064dfaf to
8875123
Compare
find_one_and_update to include Nonefind_one_and_* methods to include None
Summary
find_one_and_update's existing type hint makes the false claim that the method returns a_DocumentType. However, the docstring (correctly) tells us that the method "Returns ``None`` if no document matches the filter."The same is true of other
find_one_and_*methods.Changes in this PR
This PR corrects the type hint of the
find_one_and_*methods to indicate that the function may return either_DocumentTypeorNone(rather than just_DocumentType).I've also updated the docstrings for the methods to make this behavior clearer.
Depending on what you consider to be the "surface area" of your library's API, this might be considered a breaking change.
I can't think of any code that would break at runtime (since the PR doesn't change runtime behavior), but it's possible someone has written code like the following. This PR would cause the
assert_typeto fail at "type checking time".Testing Plan
This change isn't changing any runtime behavior, so I'm not sure how I'd test it. I'm open to suggestions for how to test this if you have any.
Screenshots (optional)
N/A
Checklist
Checklist for Author
Checklist for Reviewer {@primary_reviewer}
Focus Areas for Reviewer (optional)